fix: should revert tx when block gas limit exceeded #10770

yihuang · 2021-12-14T10:02:44Z

Description

Solution:

create a WithBranchedStore to handle state snapshot and revert
extract ConsumeBlockGasMiddleware out from RecoveryTxMiddleware.
order the middlewares properly.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

tomtau · 2021-12-14T10:05:17Z

good to back port to 0.45 @robert-zaremba @aaronc @alexanderbez

x/auth/middleware/recovery.go

alexanderbez · 2021-12-14T16:13:58Z

Also, let's add a few tests for this. Thanks @ethanfrey 🙏

yihuang · 2021-12-15T03:27:26Z

I've redone the PR in a cleaner way, by creating a SnapshotTxMiddlware outside of RecoveryTxMiddleware.

amaury1093

I like this SnapshotMiddleware solution

x/auth/middleware/snapshot.go

x/auth/middleware/middleware.go

yihuang · 2021-12-15T13:19:01Z

I'm not sure how to write a unit test for this though, it seems we need to execute some msg that does storage operations, then we can check the side-effects are reverted when the block gas limit is exceeded.

yihuang · 2021-12-15T13:22:12Z

I like this SnapshotMiddleware solution

Do you think there's a similar issue with this tip logic: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/tips.go#L38 . It's also executed after tx msgs, and it seems to be possible to return error. Maybe we should put it inside the SnapshotTxMiddleware?

alexanderbez · 2021-12-15T13:40:24Z

I'm not sure how to write a unit test for this though, it seems we need to execute some msg that does storage operations, then we can check the side-effects are reverted when the block gas limit is exceeded.

Yes, this exactly how we should test it. In fact, there might already be some BaseApp tests that do something similar.
Can we rename the new middleware? Snapshot is not indicative of what is really going on. If anything, you may call it PersistTxMiddleware

amaury1093 · 2021-12-15T14:28:08Z

Do you think there's a similar issue with this tip logic: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/tips.go#L38 . It's also executed after tx msgs, and it seems to be possible to return error. Maybe we should put it inside the SnapshotTxMiddleware?

Yes you're right. But with the current code, the TipsMiddleware is already inside SnapshotTxMiddleware (see #10770 (comment)), so all good

tomtau

looks good, just a small nit: given it's now renamed PersistTxMiddleware, perhaps the file could be persist_tx.go or persist.go instead of snapshot.go for consistency?

yihuang · 2021-12-17T01:40:51Z

looks good, just a small nit: given it's now renamed PersistTxMiddleware, perhaps the file could be persist_tx.go or persist.go instead of snapshot.go for consistency?

yeah, good catch, done.

robert-zaremba · 2022-01-07T12:03:47Z

Here is the followup issue: #10908 . It requires bigger refactor, so we won't be able to handle it in this PR unfortunately.

In this PR: let's add the missing tests and see what we can fix in the middleware we are changing.

ethanfrey · 2022-01-08T14:19:52Z

I'm looking forward to 0.45 release and this seems like the only blocker left.

Are there concrete actions needed for this? "Missing tests and fix the middleware" are a bit vague.
I have not done a thorough review, as I trust there are plenty of eyes, but if my input could help, let me know. Just want to bump this up. Nice fix to have, but looking forward to 0.45 which brings in some very important other changes

Closes: cosmos#10769 Solution: - create a `WithBranchedStore` to handle state snapshot and revert - extract `ConsumeBlockGasMiddleware` out from `RecoveryTxMiddleware`. - order the middlewares properly. Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

amaury1093 · 2022-01-10T11:58:23Z

@robert-zaremba are you okay to lift the block, and implementing more complex gas deductions/refunds in a later PR?

fedekunze · 2022-01-10T13:49:43Z

implementing more complex gas deductions/refunds in a later PR

I agree this is a nice addition but outside the scope of the PR. We'd be happy to collaborate on a design

robert-zaremba

Thanks for adding tests. I've already created a new issue for other updates discussing here.

amaury1093 · 2022-01-11T10:09:57Z

Nice, adding automerge!

I removed the backport label, there's already #10814

- used a different solution. changelog

* revert tx when block gas limit exceeded (backport: #10770) - used a different solution. changelog * add unit test * Update baseapp/baseapp.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * simplfy the defer function Co-authored-by: Robert Zaremba <robert@zaremba.ch>

…cosmos#10814) * revert tx when block gas limit exceeded (backport: cosmos#10770) - used a different solution. changelog * add unit test * Update baseapp/baseapp.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * simplfy the defer function Co-authored-by: Robert Zaremba <robert@zaremba.ch>

yihuang requested review from aaronc and alexanderbez as code owners December 14, 2021 10:02

github-actions bot added the C:x/auth label Dec 14, 2021

tomtau mentioned this pull request Dec 14, 2021

chore: v0.45.0 Release Notes #10760

Merged

20 tasks

amaury1093 assigned amaury1093 and likhita-809 Dec 14, 2021

alexanderbez suggested changes Dec 14, 2021

View reviewed changes

x/auth/middleware/recovery.go Outdated Show resolved Hide resolved

x/auth/middleware/recovery.go Outdated Show resolved Hide resolved

amaury1093 reviewed Dec 14, 2021

View reviewed changes

x/auth/middleware/recovery.go Outdated Show resolved Hide resolved

x/auth/middleware/recovery.go Outdated Show resolved Hide resolved

robert-zaremba mentioned this pull request Dec 14, 2021

Limited 0.45 Cosmos SDK release #10656

Closed

12 tasks

yihuang force-pushed the block-gas-meter-panic branch from 3760624 to 93ac439 Compare December 15, 2021 03:06

yihuang changed the title ~~Don't panic when block gas limit exceeded~~ Don't return error after tx is committed Dec 15, 2021

yihuang force-pushed the block-gas-meter-panic branch from 93ac439 to 0f263de Compare December 15, 2021 03:23

yihuang requested review from amaury1093 and alexanderbez December 15, 2021 03:24

yihuang changed the title ~~Don't return error after tx is committed~~ fix: Don't return error after tx is committed Dec 15, 2021

yihuang changed the title ~~fix: Don't return error after tx is committed~~ fix: should revert tx when block gas limit exceeded Dec 15, 2021

amaury1093 reviewed Dec 15, 2021

View reviewed changes

x/auth/middleware/snapshot.go Outdated Show resolved Hide resolved

x/auth/middleware/middleware.go Outdated Show resolved Hide resolved

yihuang force-pushed the block-gas-meter-panic branch from a834885 to 4c54a5e Compare December 15, 2021 13:17

yihuang force-pushed the block-gas-meter-panic branch 2 times, most recently from 38ff9aa to b3bb21d Compare December 16, 2021 13:24

alexanderbez approved these changes Dec 16, 2021

View reviewed changes

tomtau approved these changes Dec 17, 2021

View reviewed changes

robert-zaremba mentioned this pull request Jan 7, 2022

Fail early to consume gas if a user doesn't have enough balance #10908

Open

4 tasks

yihuang and others added 3 commits January 8, 2022 23:58

Update x/auth/middleware/branch_store_test.go

c03ab15

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

add gas overflow test case

af0cc18

yihuang force-pushed the block-gas-meter-panic branch from 1a47b58 to af0cc18 Compare January 8, 2022 16:06

github-actions bot removed the C:Store label Jan 8, 2022

robert-zaremba approved these changes Jan 11, 2022

View reviewed changes

robert-zaremba added the backport/0.45.x label Jan 11, 2022

robert-zaremba assigned robert-zaremba and unassigned likhita-809 Jan 11, 2022

amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. and removed backport/0.45.x labels Jan 11, 2022

Merge branch 'master' into block-gas-meter-panic

2f24333

mergify bot merged commit 17279fd into cosmos:master Jan 11, 2022

yihuang deleted the block-gas-meter-panic branch January 11, 2022 10:21

yihuang added a commit to yihuang/cosmos-sdk that referenced this pull request Jan 11, 2022

revert tx when block gas limit exceeded (backport: cosmos#10770)

8c9a098

- used a different solution. changelog

faddat mentioned this pull request Feb 28, 2022

v0.45.x akash-network/cosmos-sdk#176

Closed

8 tasks

yihuang mentioned this pull request Apr 15, 2022

Need an api to recover execution result from the false-failed tx evmos/ethermint#1052

Closed

amaury1093 mentioned this pull request May 12, 2022

fix!: Branch antehandlers and commit to state before runMsgs #11942

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: should revert tx when block gas limit exceeded #10770

fix: should revert tx when block gas limit exceeded #10770

yihuang commented Dec 14, 2021 •

edited

Loading

tomtau commented Dec 14, 2021

alexanderbez commented Dec 14, 2021

yihuang commented Dec 15, 2021

amaury1093 left a comment

yihuang commented Dec 15, 2021

yihuang commented Dec 15, 2021

alexanderbez commented Dec 15, 2021

amaury1093 commented Dec 15, 2021

tomtau left a comment

yihuang commented Dec 17, 2021

robert-zaremba commented Jan 7, 2022 •

edited

Loading

ethanfrey commented Jan 8, 2022

amaury1093 commented Jan 10, 2022

fedekunze commented Jan 10, 2022

robert-zaremba left a comment

amaury1093 commented Jan 11, 2022 •

edited

Loading

fix: should revert tx when block gas limit exceeded #10770

fix: should revert tx when block gas limit exceeded #10770

Conversation

yihuang commented Dec 14, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

tomtau commented Dec 14, 2021

alexanderbez commented Dec 14, 2021

yihuang commented Dec 15, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

yihuang commented Dec 15, 2021

yihuang commented Dec 15, 2021

alexanderbez commented Dec 15, 2021

amaury1093 commented Dec 15, 2021

tomtau left a comment

Choose a reason for hiding this comment

yihuang commented Dec 17, 2021

robert-zaremba commented Jan 7, 2022 • edited Loading

ethanfrey commented Jan 8, 2022

amaury1093 commented Jan 10, 2022

fedekunze commented Jan 10, 2022

robert-zaremba left a comment

Choose a reason for hiding this comment

amaury1093 commented Jan 11, 2022 • edited Loading

yihuang commented Dec 14, 2021 •

edited

Loading

robert-zaremba commented Jan 7, 2022 •

edited

Loading

amaury1093 commented Jan 11, 2022 •

edited

Loading